Skip to content

fix: builder accessor method naming conflicts#4342

Open
nated0g wants to merge 8 commits into
smithy-lang:mainfrom
nated0g:fix-builder-accessors
Open

fix: builder accessor method naming conflicts#4342
nated0g wants to merge 8 commits into
smithy-lang:mainfrom
nated0g:fix-builder-accessors

Conversation

@nated0g
Copy link
Copy Markdown

@nated0g nated0g commented Oct 10, 2025

Motivation and Context

#4338

Description

fixes a couple stray compilation errors in @haydenbaker's PR (#4341), and adds a test, adds changelog entry

Testing

Added a test in FluentClientGeneratorTest.kt to verify the bug no longer exists

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@nated0g nated0g requested review from a team as code owners October 10, 2025 18:10
@nated0g nated0g changed the title Fix builder accessors fix: builder accessor method naming conflicts Oct 10, 2025
Copy link
Copy Markdown
Collaborator

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that commit I did for the ktlint issues works this all looks good to me. Will require a review from the server team as well.

@github-actions
Copy link
Copy Markdown

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh
Copy link
Copy Markdown
Collaborator

rcoh commented Oct 16, 2025

I think this will probably cause breaking changes because setters that almost certainly exist in the wild for fields like default will end up being renamed (see the Client Test codegen diff above)

I think we need to make this context aware so that we only rename setters where the setter itself would actually conflict.

haydenbaker and others added 8 commits April 21, 2026 15:34
The setterName() extension function now requires a symbolProvider parameter.
Updated call sites to pass the correct symbolProvider.
Verifies that structures with a 'meta' field get properly renamed to
'meta_value' with corresponding setter/getter methods to avoid conflicts
with error metadata fields.
Use setterName() extension function instead of manually constructing
setter names to handle field renaming (e.g. default -> default_value).
The previous approach renamed every member that the symbol provider
touched, which changed `set_default`/`set_build`/`set_builder` (and the
matching getters) for any struct with those fields — a breaking change
flagged by @rcoh on review.

The duplicate-definition bug only occurs for error-struct builders,
where `ErrorGenerator` injects `set_meta` for `ErrorMetadata`. Only
rename members whose raw name is `meta` and leave every other setter
and getter at its pre-existing `set_<raw>` / `get_<raw>` name.

Rewrite the regression test to actually reproduce the original bug: it
now builds a `TestError` (an `@error` shape with a `meta` member) and
asserts `set_default` stays intact, so the earlier failure mode and the
guard against unrelated renames are both covered.

Also drop a now-unused `toSnakeCase` import in `ServerBuilderGenerator`
that surfaced after rebasing onto main.
@nated0g nated0g force-pushed the fix-builder-accessors branch from b7f1096 to da74b5d Compare April 21, 2026 22:39
@nated0g
Copy link
Copy Markdown
Author

nated0g commented Apr 22, 2026

I think this will probably cause breaking changes because setters that almost certainly exist in the wild for fields like default will end up being renamed (see the Client Test codegen diff above)

I think we need to make this context aware so that we only rename setters where the setter itself would actually conflict.

alright finally coming back to this because I'm tired of manually tweaking codegen output for a client I've had to regenerate a few times now 😅

@rcoh I think my latest should address your concern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants